Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @matthewjasper (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

The job x86_64-gnu-llvm-6.0 of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

2019-11-08T01:27:54.8013318Z ##[command]git remote add origin https://github.com/rust-lang/rust
2019-11-08T01:27:54.8222955Z ##[command]git config gc.auto 0
2019-11-08T01:27:54.8292509Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2019-11-08T01:27:54.8360739Z ##[command]git config --get-all http.proxy
2019-11-08T01:27:54.8522338Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/66204/merge:refs/remotes/pull/66204/merge
---
2019-11-08T01:34:13.4616936Z    Compiling serde_json v1.0.40
2019-11-08T01:34:15.4712778Z    Compiling tidy v0.1.0 (/checkout/src/tools/tidy)
2019-11-08T01:34:27.8177001Z     Finished release [optimized] target(s) in 1m 35s
2019-11-08T01:34:27.8254037Z tidy check
2019-11-08T01:34:28.4033297Z tidy error: /checkout/src/librustc_target/spec/x86_64_apple_darwin.rs:9: line longer than 100 chars
2019-11-08T01:34:30.7407509Z Found 485 error codes
2019-11-08T01:34:30.7407622Z Found 0 error codes with no tests
2019-11-08T01:34:30.7407705Z Done!
2019-11-08T01:34:30.7407798Z some tidy checks failed
2019-11-08T01:34:30.7407798Z some tidy checks failed
2019-11-08T01:34:30.7407827Z 
2019-11-08T01:34:30.7407853Z 
2019-11-08T01:34:30.7415041Z command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor"
2019-11-08T01:34:30.7415402Z 
2019-11-08T01:34:30.7415428Z 
2019-11-08T01:34:30.7420115Z failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
2019-11-08T01:34:30.7420382Z Build completed unsuccessfully in 0:01:39
2019-11-08T01:34:30.7420382Z Build completed unsuccessfully in 0:01:39
2019-11-08T01:34:30.7468249Z == clock drift check ==
2019-11-08T01:34:30.7506373Z   local time: Fri Nov  8 01:34:30 UTC 2019
2019-11-08T01:34:30.8970630Z   network time: Fri, 08 Nov 2019 01:34:30 GMT
2019-11-08T01:34:30.8986211Z == end clock drift check ==
2019-11-08T01:34:32.2932624Z 
2019-11-08T01:34:32.3043837Z ##[error]Bash exited with code '1'.
2019-11-08T01:34:32.3070638Z ##[section]Starting: Checkout
2019-11-08T01:34:32.3072934Z ==============================================================================
2019-11-08T01:34:32.3072993Z Task         : Get sources
2019-11-08T01:34:32.3073042Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

The job x86_64-gnu-llvm-6.0 of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

2019-11-08T16:12:34.1738940Z ##[command]git remote add origin https://github.com/rust-lang/rust
2019-11-08T16:12:34.1930451Z ##[command]git config gc.auto 0
2019-11-08T16:12:34.2364565Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2019-11-08T16:12:34.2422367Z ##[command]git config --get-all http.proxy
2019-11-08T16:12:34.2566970Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/66204/merge:refs/remotes/pull/66204/merge
---
2019-11-08T16:18:43.5000117Z    Compiling serde_json v1.0.40
2019-11-08T16:18:45.3114973Z    Compiling tidy v0.1.0 (/checkout/src/tools/tidy)
2019-11-08T16:18:57.0277802Z     Finished release [optimized] target(s) in 1m 30s
2019-11-08T16:18:57.0357660Z tidy check
2019-11-08T16:18:57.6019596Z tidy error: /checkout/src/librustc_target/spec/x86_64_apple_darwin.rs:9: line longer than 100 chars
2019-11-08T16:18:59.8162461Z Found 485 error codes
2019-11-08T16:18:59.8163299Z Found 0 error codes with no tests
2019-11-08T16:18:59.8163550Z Done!
2019-11-08T16:18:59.8163885Z some tidy checks failed
2019-11-08T16:18:59.8163885Z some tidy checks failed
2019-11-08T16:18:59.8170255Z 
2019-11-08T16:18:59.8170370Z 
2019-11-08T16:18:59.8172001Z command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor"
2019-11-08T16:18:59.8172460Z 
2019-11-08T16:18:59.8172489Z 
2019-11-08T16:18:59.8178195Z failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
2019-11-08T16:18:59.8179447Z Build completed unsuccessfully in 0:01:34
2019-11-08T16:18:59.8179447Z Build completed unsuccessfully in 0:01:34
2019-11-08T16:18:59.8229667Z == clock drift check ==
2019-11-08T16:18:59.8246106Z   local time: Fri Nov  8 16:18:59 UTC 2019
2019-11-08T16:18:59.9155856Z   network time: Fri, 08 Nov 2019 16:18:59 GMT
2019-11-08T16:18:59.9159091Z == end clock drift check ==
2019-11-08T16:19:01.1952144Z 
2019-11-08T16:19:01.2050815Z ##[error]Bash exited with code '1'.
2019-11-08T16:19:01.2079662Z ##[section]Starting: Checkout
2019-11-08T16:19:01.2081852Z ==============================================================================
2019-11-08T16:19:01.2081912Z Task         : Get sources
2019-11-08T16:19:01.2081962Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

r? @alexcrichton

After asking on Discord, someone suggested that @alexcrichton was the right person to look at this kind of fix.

Changing the default set of linker flags for a target is unfortunately no small task. Linkers are very finnicky and are used all the time so we don't tend to know what we're relying on when calling linkers. I say this in the sense that this has an unusually high risk of causing accidentally regression due to tweaking how the linker is being invoked. I do not know of possible regressions, but they almost always happen when changing linker arguments.

I also don't really know what we should be doing here about undefined symbols. This runs the risk of papering over real issues in addition to fixing your original issue. For example maybe a symbol is accidentally undefined?

At this time I don't think we have a real great way of evaluating this change to be confident in it one way or another. This sort of requires someone with deeper knowledge of linking on macOS to know whether we should be passing this by default and what our possible risks of regressions are. Unfortunately I'm not that person.

This sort of requires someone with deeper knowledge of linking on macOS to know whether we should be passing this by default and what our possible risks of regressions are. Unfortunately I'm not that person.

Neither am I, unfortunately. Any idea who I might reach out to?

How are linker changes normally made and what kind of fallout have we seen in the past? Are we worried about compilation failures, or some subtle performance regression due to a slightly different linking process? Does the CI test suite have reasonable platform coverage to boost confidence, and if so, how do I kick off a run that has my patch in it?

I also don't really know what we should be doing here about undefined symbols. This runs the risk of papering over real issues in addition to fixing your original issue. For example maybe a symbol is accidentally undefined?

I understand why you want some more expertise involved in fixing this; but it's pretty clear to me that this is a bug and should be fixed.

Take a look at libssl:

$ objdump -t  /usr/local/opt/openssl/lib/libssl.1.0.0.dylib|grep '_X509_verify_cert$'
0000000000000000         *UND*	_X509_verify_cert
$ objdump -t  /usr/local/opt/openssl/lib/libcrypto.dylib|grep '_X509_verify_cert$'
00000000000c8200 g     F __TEXT,__text	_X509_verify_cert
$ otool -L  /usr/local/opt/openssl/lib/libssl.1.0.0.dylib
/usr/local/opt/openssl/lib/libssl.1.0.0.dylib:
	/usr/local/opt/openssl/lib/libssl.1.0.0.dylib (compatibility version 1.0.0, current version 1.0.0)
	/usr/local/Cellar/openssl/1.0.2s/lib/libcrypto.1.0.0.dylib (compatibility version 1.0.0, current version 1.0.0)
	/usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1252.250.1)

Notice that libssl references, but does not define, _X509_verify_cert. That's actually defined in libcrypto. The process of loading libssl also loads its dependency, libcrypto.

The point here is that it's fundamental and expected that a shared library might reference symbols in other libraries or the host program. Apparently, that is not common in the rust world, but I don't see a reason it shouldn't be supported in a low-level language like rust for a crate type called cdylib.

If we want to catch mistakes, relying on platform-specific linking details is the wrong way to do it anyway. It would be much more reliable to simply try loading the library, which would catch unresolved symbols on any platform. If there is a use case beyond that, we could expose a feature on all platforms that will explicitly check for unresolved symbols.

@aleksijuvani are you the right person to take a look here?

I'm not familiar with this linker flag, sorry!

@froydnj any thoughts?

Unfortunately I do not know of someone myself with sufficient experience to be able to review this. We historically just basically haven't ever changed the linker flags for tier 1 targets (unless it only applies in niche situations). Also unfortunately our test suite would not provide much coverage here in terms of regression testing.

I am personal wary of this change because of how big this hammer is. It basically says "stop ever reporting undefined symbol errors on macOS". Some questions I would have would be:

• What happens on other platforms? (windows/linux)
• What do C projects typically do? Do they always compile with this flag? What is their recommendation when they're included statically elsewhere?
• Is there a way for this to be less of a "hammer" approach and be slightly more targeted?

I personally feel that just knowing more about this change will help boost confidence in it. At this time it, to me at least, still feels like too much of an unknown "whack this thing with that hammer" fix for me to personally have confidence in it.

What happens on other platforms? (windows/linux)

On Linux, the default for undefined symbols in executables is to error; the default for undefined symbols in shared libraries is to allow them.

I believe, but am not certain, that the default on Windows is to reject undefined symbols always.

Notice that libssl references, but does not define, _X509_verify_cert. That's actually defined in libcrypto. The process of loading libssl also loads its dependency, libcrypto.

Yes, this is expected: the runtime loader on OS X will load libcrypto prior to attempting to resolve symbols in libssl. But I suspect that when libssl is being compiled, the final linking step to create the shared library instructs the linker to search libcrypto for symbols (i.e. -lcrypto), rather than setting -undefined dynamic_lookup`. It's kind of unusual (on OS X, anyway, the situation is slightly different on Linux, and really only for certain libraries) for compilation of a shared library to include symbols that don't get resolved against any of the libraries the shared library is linked against, but that will somehow be provided when the shared library is actually used.

What is the situation where -undefined dynamic_lookup looks to be the appropriate solution?

If we want to catch mistakes, relying on platform-specific linking details is the wrong way to do it anyway. It would be much more reliable to simply try loading the library, which would catch unresolved symbols on any platform. If there is a use case beyond that, we could expose a feature on all platforms that will explicitly check for unresolved symbols.

The linker can already determine that all the symbols in your shared library are defined in the library itself, or defined in other shared libraries that your shared library links to/depends on. This behavior is just as reliable as loading the library itself.

Given that -undefined error is the default behavior on OS X, and in the absence of evidence of widespread use of -undefined dynamic_lookup, I'd be inclined to not change Rust's behavior here, and instead require modifying linker flags to be done through build.rs or -C link-arg= in command-line options/environment variables. (I am not a Rust maintainer, though.)

What is the situation where -undefined dynamic_lookup looks to be the appropriate solution?

PostgreSQL allows the loading of extensions, which are some combination of SQL and a shared library. For non-trivial extensions, it's common for the shared library to call internal postgres APIs, which are defined in the PostgreSQL executable.

I am developing https://github.com/jeff-davis/postgres-extension.rs to make it possible to develop extensions in rust, but this is one of the roadblocks I'm hitting.

Given that -undefined error is the default behavior on OS X, and in the absence of evidence of widespread use of -undefined dynamic_lookup, I'd be inclined to not change Rust's behavior here, and instead require modifying linker flags to be done through build.rs or -C link-arg= in command-line options/environment variables. (I am not a Rust maintainer, though.)

My crate is just meant to make it possible for others to develop extensions in rust, using my crate as a dependency. So I think any build hack needs to be done by all users of my crate rather than just in my crate. Perhaps there's a way around that or maybe I should try to document that everyone needs to do that. But at least right now, it seems like that build hack would spill over to users of my crate.

I am developing https://github.com/jeff-davis/postgres-extension.rs to make it possible to develop extensions in rust, but this is one of the roadblocks I'm hitting.

Thank you for the additional context! I can see how undefined symbols are definitely the expected behavior here. I suppose that building Postgres extensions on Mac in C/C++ also requires -undefined dynamic_lookup, then?

My crate is just meant to make it possible for others to develop extensions in rust, using my crate as a dependency. So I think any build hack needs to be done by all users of my crate rather than just in my crate. Perhaps there's a way around that or maybe I should try to document that everyone needs to do that. But at least right now, it seems like that build hack would spill over to users of my crate.

This is absolutely a reasonable concern! But I think if you have a build.rs in your crate that does (warning, completely untested code here):

fn main() {
  // Permit undefined symbols to Postgres internal symbols in the `postgres` binary.
  // https://doc.rust-lang.org/cargo/reference/build-scripts.html#outputs-of-the-build-script
  println!("cargo:rustc-cdylib-link-arg=-undefined");
  println!("cargo:rustc-cdylib-link-arg=dynamic_lookup");
}

I think that any crate that depends on your crate magically inherits the above flags when a shared library is created.

Thank you for the additional context! I can see how undefined symbols are definitely the expected behavior here. I suppose that building Postgres extensions on Mac in C/C++ also requires -undefined dynamic_lookup, then?

Third party extensions, like PostGIS, use -undefined dynamic_lookup on mac. I was going to include a link to the source code, but it seems like it's in the release tarball but not the source code. Trying to figure out which packaging step puts it in the tarball.

Extensions that come with postgres, like hstore, use -bundle_loader ../../src/backend/postgres.

I think that any crate that depends on your crate magically inherits the above flags when a shared library is created.

I tried that, but it seems like it needs to be in the shared library's crate. If I move it up to the postgres-extension.rs crate, it doesn't seem to be working.

I tried that, but it seems like it needs to be in the shared library's crate. If I move it up to the postgres-extension.rs crate, it doesn't seem to be working.

Hmmm, interesting! I would have expected it to, but IIUC, this bit in cargo only triggers for the current crate being compiled:

https://github.com/rust-lang/cargo/blob/e55600b1ddb9d1ee0ed692e37172798216634ace/src/cargo/core/compiler/mod.rs#L362-L367

Rather than changing the defaults for every Rust program, maybe it's worth exploring whether cargo could be changed to propagate linker args upwards in the dependency graph by moving that block outside of the key.0 == current_id condition?

Alternatively, I guess postgress-extension could document some helper function that's necessary to call in your build script (and therefore some build-dependency crate), but that is not quite as elegant.

Thanks for the comments here @froydnj! I think I agree that exploring a Cargo/build system solution here is probably best rather than changing the default behavior in the compiler.

It looks like glibtoolize on Mac is generating the -undefined dynamic_lookup flag as part of macros/libtool.m4 (a file generated by glibtoolize). That seems to suggest it's not a strange/uncommon thing.

I created rust-lang/cargo#7612 to represent the cargo change @froydnj suggested. I think that's independently useful as a way to enable linker hacks when necessary, so I support that change.

I'm not 100% sure we should give up on this rustc change, though. I've updated this PR here so that the linker flag only applies when creating a cdylib, which should reduce the blast radius. Perhaps we can find a Mac linking expert to give some better input on what the right thing to do here is.

The other PR ( rust-lang/cargo#7612 ) seems unsatisfying and is not necessarily making progress.

We have a clear use case, problem, and fix on the table with this current PR. My expectation for a supported platform is that we should be able to fix platform bugs, or at least do some investigation to learn why we can't fix it or why the proposed fix is wrong.

I haven't received any feedback on what's wrong with this PR other than a general discomfort with making any changes to the linking process at all. If we see some evidence that this fix is a minefield, then I guess we need to resort to hacks and workarounds. But everything I've seen so far suggests that this flag is a routine solution to this problem.

I have made some attempts to boost our confidence by looking at other projects and build tools, and I have also changed my PR to narrow the scope and hopefully put the flag in a more suitable place.

More looking around shows that others have run into this problem as well, such as:

• https://github.com/PyO3/pyo3/blob/935adaa5d5220b4fbeff36fd02a7ea59e8144b4c/README.md
• https://metacpan.org/release/Module-Install-Rust/source/lib/Module/Install/Rust.pm#L190

...and solved it by hacking the build to use the same -undefined dynamic_lookup flag.

Sure, I can hack around the problem in similar ways, but I don't see why any of it is necessary. It's just a way for a developer to make a mistake, forget to test on MacOS, and then their crate breaks.

I'm sorry this isn't a great situation, and I agree it's definitely not great! I will stand by what I said above though, in that I do not personally have the confidence to r+ this change as is, nor do I know who can. That's basically just the fact-of-the-matter right now, and while it'd be great if that could change I don't have the time and energy right now to build up all the necessary context to sign off on a change like this.

Doing a conservative change is going to be difficult in Cargo, as I've listed on that PR.

If you want to land this change as-is you'll likely want to get in touch with the compiler team on Zulip and see if someone there can help take over review. Someone else may have the time to help confirm and/or investigate more here to see if this is a reasonable change to make.

r? @parthsane

Sorry, I am not familiar with linking in mac. I don't think I can help.

r? @Centril

r? @michaelwoerister cc @Zoxc

Sorry, my knowledge about dynamic linking on macOS is rather limited.

Nominating for T-Compiler to find a suitable reviewer.

This seems to me like a niche platform specific use case which should not be the default for rustc. If you want this behavior you should have to opt in for it.

The use case is certainly not platform-specific. I only found it when I later tried to build my crate on Mac. I think a better description is that the problem is platform-specific.

I posted links to other projects that also need to do this hack (e.g. creating a loadable module for python in rust), so I'm not completely alone.

I don't understand your comment that it "should not be the default for rustc". Right now it does exactly what I want on linux: it allows undefined symbols in shared libraries. Are you suggesting that we should change linking on linux so that it also rejects undefined symbols unless you opt-in?

I would understand the point better if rustc generally didn't take much ownership of the linking process (like a C compiler). But it already does -- my PR just adds a new flag next to another MacOS-specific linker flag.

Hmm. So we discussed this in the compiler meeting. Speaking personally, I would say a few things:

This is a tricky issue! I appreciate @jeff-davis's frustration but I also feel like there is good reason to be cautious. Adding this flag to the default for Mac OS is not something we can undo, after all, and for the most part I believe our linker flags are fairly "conservative" (i.e., they stick to the defaults from the linker itself).

One thing that I think would be useful is trying to collect some of the information in this thread. I found I had to read it 2 or 3 times to extract out the various points. I think the summary looks something like:

• On Linux, the default for executables is to error on undefined symbols, but the default in shared libraries is to permit them and expect them to be found later (same as this PR). I believe that is just the default linker behavior, right, not due to some flags that we add? (Citation)
• On Windows, the default is always to error (Citation) -- this could use verification
• On Mac, the current default is always to error, but this PR changes it to match Linux
• In the particular use case of creating Postgres extensions, it is expected that the extensions may link to internals of Postgres that are not available until runtime
• There are some other projects which add this flag, such as pyo3 and something from CPAN. I gather that these projects are also aiming to create dynamically loaded modules in similar scenarios.
• You can pass the flag manually, but in this case the project is more of a dependency that others rely on, so each "final project" would ultimately have to add their own build.rs with some pre-canned lines of code. Still, that seems like at least a viable workaround.
  • An alternate strategy might be to extend cargo with the ability to propagate flags upward, and there is an extent PR pursuing that, but it seems complicated.

Did I miss anything? Looking at the above summary, a couple of questions arise:

• If I'm correct, our behavior would now be inconsistent for Windows...is this a potential issue?
• Do projects like Xcode have this behavior?
• Are there potential side-effects to enabling this linking mode -- i.e., can it change the behavior of anything that works today?

I think what we're missing is more feedback from people deeply familiar with the Mac platform, who might be able to say if this is a pretty standard thing to do or what. I guess it feels to me like changes of this kind, even if they are only one line of code, might be more appropriately handled with an RFC, since there are potentially ramifications that are hard for us to fully assess.

Thank you @nikomatsakis for an excellent summary.

I will attempt an RFC for "Defining the behavior of unresolved symbols in cdylib crates".

triage: @nikomatsakis 's comment summarized the T-compiler discussion nicely. I'm going to remove nomination (since we have discussed this) and close the PR (since we think this, at the very least, requires an RFC.)